Skip to content

feat(health): report schema version skew instead of failing silently - #178

Merged
zaridan merged 2 commits into
mainfrom
feat/schema-skew-diagnostic
Aug 2, 2026
Merged

feat(health): report schema version skew instead of failing silently#178
zaridan merged 2 commits into
mainfrom
feat/schema-skew-diagnostic

Conversation

@zaridan

@zaridan zaridan commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🟢 SAFE TO MERGE

Gates green. Decision approved. Review adjudicated.

Codex (adversarial, in place of CodeRabbit): 2 findings, 2 real, both fixed.

⚠️ CodeRabbit never reviewed this PR — rate-limited on every attempt ("Review limit reached… we couldn't start this review"). Its check reads pass because it was refused, not because it looked. Not re-requested further: repeated requests push the reset window out, not in.

What the substitute pass found — both in my own design

Severity Finding Fix
High The diagnostic failed the exact case it was built for. The schema check ran last, so on an empty database the queue query threw first and the operator got a bare relation "queue_jobs" does not exist 500 — no cause, no command. Confirmed by running it, not by reading it. Schema check runs first; traffic checks wrapped so a failure they cause is reported as the schema problem, with an explicit health-checks-unavailable note rather than reporting those sections healthy
Medium max(id) called a gapped history healthy. A database holding 1–26 plus 29, missing 27 and 28, has max = 29 — equal to the build's latest Compares the full id set and names the missing ids

The test for the High finding proved nothing. It migrated fully, then dropped only _migrations, leaving every other table in place — so it passed while the real code was broken. It now uses a genuinely empty database.

Accepted and deliberately unchanged: a skew returns 503, so an operator running npm run migrate after a deploy pages a monitor for the duration. That is a real skew, and saying so is the point.

Makes the deploy-without-migrate window announce itself. Prerequisite for turning on auto-deploy (#176).

Decision provenance

Decision — in plain words Source
Build this before enabling auto-deploy on merge You, 2026-08-02: "yes, build the skew diagnostic"
It reports the problem; it does not migrate anything Preserves scripts/migrate.ts's existing "schema changes are an operator step" rule — not a new call

The problem it solves

Observed in production earlier today: HT-101 shipped three migrations, the deploy landed first, and imap-fetch spent the gap erroring every two minutes against tables that did not exist. Nothing named the cause. The only symptom was a failing cron.

With auto-deploy on merge (#176) that window opens on every merge, without anyone choosing to open it.

What it does

GET /api/v1/internal/health now compares this build's LATEST_MIGRATION_ID against max(_migrations.id):

State Alert Result
Database behind the build schema-migration-pending 503, message names both versions and the command
Database ahead of the build schema-newer-than-build 503 — usually a rollback, never fine-and-intentional
No _migrations table schema-migration-pending 503 saying "never been migrated"
Matched silent

Example body on a skew:

schema-migration-pending: database is at migration 28, this build expects 29. Run npm run migrate. Until then, code paths using the newer schema will fail.

Actionable without opening the repo, which is the point — the existing endpoint already returns 503 on any alert, so any status-code monitor picks this up with no new wiring.

Design notes

  • LATEST_MIGRATION_ID is derived from MIGRATIONS, not written down, so it cannot drift from the list it describes.
  • It does not migrate. Reporting the skew keeps the operator-step rule intact while removing the part that made it dangerous: silence.
  • Table existence is checked in its own statement. A single CASE WHEN to_regclass(…) … ELSE (SELECT max(id) FROM _migrations) does not work — Postgres resolves the relation at parse time, so the subquery errors before the CASE short-circuits, turning "never migrated" into a generic 500. My first version had exactly that bug; the test written for that case caught it.

Verification

Gate Result
npm run typecheck exit 0
npm run -w web typecheck exit 0
npm run -w web build exit 0
npm run lint exit 0
npm test exit 0 — 87 files, 1,746 tests
gitleaks exit 0

Four new tests cover all four states, and assert the message contains both version numbers and npm run migrate — so a future refactor cannot quietly make the alert useless while still tripping.

Not in scope

Auto-deploy itself is still off: both Vercel projects have no Git repository connected (link: null), and the Vercel GitHub App is not installed on the Helpthread org. That needs a browser click — see #176.

`src/composition/root.ts` deliberately does not migrate on cold start —
schema changes are an operator step (`scripts/migrate.ts`). Every deploy
therefore opens a window where the new build runs against the previous schema
until someone runs `npm run migrate`, and with auto-deploy-on-merge that window
opens without anyone choosing to open it.

Observed in production 2026-08-02: HT-101 shipped three migrations, the deploy
landed first, and the `imap-fetch` cron spent the gap erroring every two minutes
against tables that did not exist yet. Nothing named the cause — the only
symptom was a failing cron, one request at a time.

`/api/v1/internal/health` now compares the build's own `LATEST_MIGRATION_ID`
against `max(_migrations.id)` and trips:

- `schema-migration-pending` — the database is BEHIND this build. The message
  names both versions and the command to run, so a 503 body is actionable on
  its own without opening the repo.
- `schema-newer-than-build` — the database is AHEAD. Usually a rollback, never
  intentional-and-fine, so it is said out loud rather than passed.

`LATEST_MIGRATION_ID` is derived from `MIGRATIONS` rather than written down, so
it cannot drift from the list it describes.

This deliberately does NOT migrate anything. Reporting the skew keeps the
"schema changes are an operator step" rule intact while removing the part that
made it dangerous: that the violation was silent.

Table existence is checked in its own statement. A single
`CASE WHEN to_regclass(...) IS NULL ... ELSE (SELECT max(id) FROM _migrations)`
does not work — Postgres resolves the relation at parse time, so the subquery
errors before the CASE can short-circuit, turning "you have never migrated"
(the likeliest first-run state) into a generic 500. Caught by the test written
for exactly that case.

Gates: typecheck, web typecheck, web build, lint, gitleaks all exit 0;
87 files / 1746 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@zaridan, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4acc9298-4b25-4013-bcc8-48135f879293

📥 Commits

Reviewing files that changed from the base of the PR and between 8c42a6b and b6cd075.

📒 Files selected for processing (4)
  • src/composition/app.test.ts
  • src/composition/health.test.ts
  • src/composition/health.ts
  • src/db/migrate.ts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Adversarial Codex pass (CodeRabbit rate-limited on this head). Two real
findings, both in the previous commit's own design.

**The diagnostic failed the exact case it was built for.** `runHealthCheck`
queried application tables before `_migrations`, so on a genuinely empty
database the queue query threw first and the operator got
`relation "queue_jobs" does not exist` as a bare 500 — no cause, no
`npm run migrate`. Confirmed by running it against a fresh PGlite instance
before fixing.

The test that was supposed to cover this proved nothing: it migrated fully,
then dropped only `_migrations`, leaving every other table in place. It now
uses a genuinely empty database and asserts the diagnostic.

The check is now first, and the traffic checks are wrapped: if they throw AND
the schema is behind, the schema alert is returned along with an explicit
`health-checks-unavailable` note, rather than reporting those sections as
healthy. If the schema is in step, a failure is a real fault and still
propagates.

**`max(id)` called a gapped history healthy.** A database holding 1..26 plus 29
while missing 27 and 28 has `max(id) === 29`, which equalled the build's latest
and reported fine. The check now compares the full set (`MIGRATION_IDS`) and
names the missing ids. `migrate()`'s single transaction makes that state
unreachable through the normal path — manual repair and hand-edited bookkeeping
are precisely what a health check is for.

Also reported and accepted as intended, not changed: a skew produces 503, so an
operator running `npm run migrate` after a deploy will page a monitor for the
duration of the migration. That is a real skew and saying so is the point.

Gates: typecheck, web typecheck, web build, lint all exit 0; 87 files /
1748 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@zaridan
zaridan merged commit 4adde52 into main Aug 2, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant